Skip to content

Extract query ID from all kill_query procedure variations #425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 30, 2024
Merged

Extract query ID from all kill_query procedure variations #425

merged 3 commits into from
Oct 30, 2024

Conversation

@willmostly
Copy link
Contributor

@willmostly willmostly commented Jul 29, 2024

Description

Trino Gateway attempts to extract the query id from the body of queries using the runtime.kill_query procedure from the system catalog. This adds a test for this functionality.

Additional context and related issues

Release notes

( x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Jul 29, 2024
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 0f19b85 to 1ad0e45 Compare July 29, 2024 19:15
mosabua
mosabua approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine .. I wish we would not have to mock around for stuff like that but a full integration test seems worse..

ebyhr
ebyhr reviewed Aug 24, 2024
View reviewed changes
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 1ad0e45 to a8355b6 Compare August 30, 2024 21:42
@willmostly willmostly requested a review from ebyhr August 30, 2024 21:45
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from a8355b6 to 83760bd Compare August 30, 2024 21:53
ebyhr
ebyhr reviewed Aug 30, 2024
View reviewed changes
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 9a00f3e to 95e782a Compare September 3, 2024 19:40
@willmostly willmostly changed the title Test extraction of query ID from kill_query procedure Extract query ID from all kill_query procedure variations Sep 3, 2024
@willmostly willmostly requested a review from ebyhr September 3, 2024 19:43
ebyhr
ebyhr reviewed Sep 3, 2024
View reviewed changes
ebyhr
ebyhr previously requested changes Sep 4, 2024
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regular expression is incorrect in some cases.

@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 95e782a to c311199 Compare September 4, 2024 20:10
@willmostly willmostly requested a review from ebyhr September 4, 2024 20:13
@mosabua
Copy link
Member

mosabua commented Sep 5, 2024

Needs a rebase now..

@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from c311199 to e9226b6 Compare September 9, 2024 19:29
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch 2 times, most recently from 805fd81 to e805660 Compare September 23, 2024 19:05
@willmostly
Copy link
Contributor Author

willmostly commented Sep 23, 2024

@ebyhr I switched to using the full query parser, which should be resilient to whitespace, comments, the string literals containing the procedure name, etc

@willmostly willmostly requested a review from ebyhr September 23, 2024 19:10
ebyhr
ebyhr reviewed Sep 23, 2024
View reviewed changes
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from e805660 to 6e9842a Compare September 25, 2024 23:56
ebyhr
ebyhr requested changes Sep 26, 2024
View reviewed changes
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 6e9842a to 76f2c68 Compare September 26, 2024 13:05
ebyhr
ebyhr reviewed Sep 26, 2024
View reviewed changes
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 76f2c68 to f4ee990 Compare September 26, 2024 20:57
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from cb54a46 to b8ed756 Compare October 16, 2024 00:38
@willmostly willmostly requested a review from ebyhr October 16, 2024 00:38
ebyhr
ebyhr reviewed Oct 16, 2024
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash the 3rd commit into the 1st commit.

@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch 2 times, most recently from 2e2cc3e to 25359b1 Compare October 16, 2024 23:00
ebyhr
ebyhr reviewed Oct 16, 2024
View reviewed changes
gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java Outdated
this.schemas = requireNonNullElse(schemas, ImmutableSet.of());
this.catalogSchemas = requireNonNullElse(catalogSchemas, ImmutableSet.of());
this.isNewQuerySubmission = isNewQuerySubmission;
this.procedureArguments = requireNonNullElse(procedureArguments, ImmutableList.of());
Copy link
Member

@ebyhr ebyhr Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does procedureArguments become null?

Copy link
Contributor Author

@willmostly willmostly Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

procedureArguments will be null if TrinoQueryProperties is constructed from a json that does not include this field. If the field is missing, the constructor will be called with a null argument, except for the Optional fields such as procedure. I will add a test case.

Copy link
Member

@ebyhr ebyhr Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you added testJsonMissingFields test. The question is what's the scenario we pass {} json to TrinoQueryProperties?

Copy link
Contributor Author

@willmostly willmostly Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether or not an empty array is serialized depends on the configuration of the ObjectMapper in JsonCodec right? If airlift decides to set .setSerializationInclusion(JsonInclude.Include.NON_DEFAULT) then these fields won't be included.

I will remove the check if that scenario can be ignored.

@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 33103ec to 714f415 Compare October 17, 2024 02:50
@willmostly willmostly requested a review from ebyhr October 17, 2024 03:03
ebyhr
ebyhr reviewed Oct 17, 2024
View reviewed changes
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 5043e6c to 3b54ac9 Compare October 17, 2024 13:08
@willmostly willmostly requested a review from ebyhr October 18, 2024 13:42
@mosabua
Copy link
Member

mosabua commented Oct 24, 2024

Can we get this in now @ebyhr and @willmostly so we can ideally cut 12?

@ebyhr
Copy link
Member

ebyhr commented Oct 24, 2024

@mosabua No, the review hasn't yet completed.

ebyhr
ebyhr requested changes Oct 25, 2024
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I strongly disagree with the current approach because we have to fight with serialization issues of procedure arguments. The maintainability seems so bad.

Please consider different approach. For instance, a) adding Optional<String> queryId to TrinoQueryProperties and setting the value during parse. b) extracting a helper method which retrieves query id from kill_query procedure without relying on TrinoQueryProperties.

I remember you mentioned that TrinoQueryProperties class is used because some other information might be helpful in the future. This is assumption, not required for now. I don't think we want to handle in this PR.

@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch 2 times, most recently from 98b634d to 893dcaf Compare October 25, 2024 18:53
@willmostly
Copy link
Contributor Author

willmostly commented Oct 25, 2024

Now I strongly disagree with the current approach because we have to fight with serialization issues of procedure arguments. The maintainability seems so bad.

I agree, having access to the procedure and arguments for routing was not worth it. I took the approach of extracting the queryId from kill_query during parsing. It isn't serialized because if a query ID is found, routing rules will not be invoked.

ebyhr
ebyhr reviewed Oct 27, 2024
View reviewed changes
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from 893dcaf to ae59adc Compare October 28, 2024 20:39
@willmostly willmostly force-pushed the will/test_extract_query_id_from_system_runtime_kill_query branch from ae59adc to b274916 Compare October 28, 2024 20:40
@willmostly willmostly requested a review from ebyhr October 29, 2024 17:22
ebyhr
ebyhr approved these changes Oct 29, 2024
View reviewed changes
gateway-ha/src/main/java/io/trino/gateway/ha/handler/ProxyUtils.java
}
else {
queryId = tokens[1];
return Optional.of(tokens[1]);
Copy link
Member

@ebyhr ebyhr Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove redundant else: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#additional-ide-configuration

Copy link
Member

@oneonestar oneonestar Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#541

@mosabua mosabua merged commit 2add137 into trinodb:main Oct 30, 2024
2 checks passed
@github-actions github-actions bot added this to the 12 milestone Oct 30, 2024
@mosabua mosabua mentioned this pull request Oct 31, 2024
Merged
@oneonestar oneonestar mentioned this pull request Oct 31, 2024
Merged
@Chaho12 Chaho12 mentioned this pull request Jun 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oneonestar oneonestar oneonestar left review comments

@mosabua mosabua mosabua approved these changes

@ebyhr ebyhr ebyhr approved these changes

Assignees

No one assigned

Labels

cla-signed

Milestone

12

Development

Successfully merging this pull request may close these issues.

4 participants

@willmostly @mosabua @ebyhr @oneonestar